-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add workspace pkg for managing local configuration #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was super easy to follow, I really liked the comments you added. It feels like effort was put into it :)
small nits
pkg/workspace/service.go
Outdated
} | ||
|
||
func (ws *Service) manifestDirectoryPath() (string, error) { | ||
if ws.manifestDir != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we add a comment here saying that we're caching the manifest dir path to avoid repeating system calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's better to call this function in the constructor and then have all the other functions just refer to ws.manifestDir
? this way we won't have to do the additional error check in the other functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that's not a bad idea. sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that might be a little tough because of the create case, when we expect there to not be a root directory yet =/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to use pkg/error to wrap all our errors so that when things break we have the full stacktrace instead of just a single error?
pkg/workspace/workspace.go
Outdated
|
||
// If there isn't an existing workspace summary, create it. | ||
var noProjectFound *ErrNoProjectAssociated | ||
if errors.As(existingWorkspaceSummaryErr, &noProjectFound) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The future is here
pkg/archer/workspace.go
Outdated
} | ||
|
||
// ManifestManager can read, write and list local manifest files. | ||
type ManifestManager interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this interface be simplified to:
type Manifest interface {
Write([]byte, string) error
Read(string) ([]byte, error)
List() ([]string, error)
}
Manager
seems implicit via the API.
pkg/workspace/errors.go
Outdated
@@ -0,0 +1,43 @@ | |||
package workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing Copyright lines
pkg/workspace/workspace_test.go
Outdated
@@ -0,0 +1,379 @@ | |||
package workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright lines
Adding a new package for easy reading and writing of local workspace level configuration. This includes: 1. Reading and Writing manifest files 2. Reading and Writing a project breadcrumb file The project breadcrumb is used to store the project name (and in the future perhaps the pipeline in charge of this workspace).
Adding a new package for easy reading and writing of local
workspace level configuration.
This includes:
The workspace summary is used to store the project name
(and in the future perhaps the pipeline in charge of this
workspace).